Complement variables, rationalize coefficients and scale in order to find knapsack cuts. Lift knapsack cuts#970
Open
chris-maes wants to merge 6 commits intoNVIDIA:release/26.04from
Open
Conversation
chris-maes
added
improvement
Contributor
Author
|
/ok to test daad714 |
📝 WalkthroughWalkthroughRemoved Root LP dual objective logging and made cut-pool type printing unconditional. Refactored knapsack cut generation: renamed API, moved rational/gcd/lcm helpers to free functions, added rationalization, complementation state, minimal-cover partitioning, lifting, and new knapsack helper methods/state. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
2223-2227:⚠️ Potential issue | 🟠 MajorGuard cut-pool diagnostics behind debug logging.
Line 2223 forces this block on in all builds (
#if 1), so every cut pass prints unconditionally. In this hot path, that can materially increase runtime/log volume, andprintfon Line 2226 bypasses logger controls.🔧 Suggested fix
-#if 1 +#ifdef CUOPT_LOG_DEBUG cut_pool.print_cutpool_types(); print_cut_types("In LP ", cut_types, settings_); - printf("Cut pool size: %d\n", cut_pool.pool_size()); + settings_.log.debug("Cut pool size: %d\n", cut_pool.pool_size()); `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 2223 - 2227, This diagnostic block is unconditionally enabled via "#if 1" and uses printf, so wrap the cut_pool.print_cutpool_types(), print_cut_types("In LP ", cut_types, settings_), and the pool size output behind the project's debug/logging guard (e.g., check a debug flag or Logger::is_debug_enabled()) instead of "#if 1"; also replace the printf call with the project's logger (e.g., processLogger.debug/info) so it respects log levels and formatting; specifically, remove the "#if 1" block, add a conditional like "if (debug_enabled)" around calls to cut_pool.print_cutpool_types(), print_cut_types(...), and the pool-size output, and use the logger API rather than printf when emitting the pool size.cpp/src/cuts/cuts.cpp (1)
725-744:⚠️ Potential issue | 🟠 MajorRemove unconditional knapsack logging from the hot path.
Line 725 (
if (1)) and Line 741 (#if 1) force verbose logging for every qualifying row, which can significantly slow separation and flood logs on large MIPs.♻️ Proposed fix
- if (1) { + if (verbose) { settings.log.printf( "Knapsack constraint %d row len %d beta %e sum_neg %e sum_pos %e sum_pos / (row_len - 1) %e\n", i, row_len, beta, sum_neg, sum_pos, sum_pos / (row_len - 1)); } @@ -#if 1 - i_t num_knapsack_constraints = knapsack_constraints_.size(); - settings.log.printf("Number of knapsack constraints %d\n", num_knapsack_constraints); -#endif + if (verbose) { + i_t num_knapsack_constraints = knapsack_constraints_.size(); + settings.log.printf("Number of knapsack constraints %d\n", num_knapsack_constraints); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.cpp` around lines 725 - 744, The code currently forces verbose logging in the knapsack detection hot path via the unconditional if (1) that calls settings.log.printf and the `#if` 1 block printing num_knapsack_constraints; remove these unconditional logs and instead wrap both the per-row printf and the final count printf in a runtime verbosity or debug flag check (e.g., settings.verbosity() > N or settings.log.isDebugEnabled()) so logging only occurs when enabled, and avoid computing expensive expressions (like sum_pos / (row_len - 1)) unless the log will be emitted; update the blocks around knapsack_constraints_, the per-row settings.log.printf call, and the num_knapsack_constraints print to use the conditional verbosity guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2223-2227: This diagnostic block is unconditionally enabled via
"#if 1" and uses printf, so wrap the cut_pool.print_cutpool_types(),
print_cut_types("In LP ", cut_types, settings_), and the pool size output
behind the project's debug/logging guard (e.g., check a debug flag or
Logger::is_debug_enabled()) instead of "#if 1"; also replace the printf call
with the project's logger (e.g., processLogger.debug/info) so it respects log
levels and formatting; specifically, remove the "#if 1" block, add a conditional
like "if (debug_enabled)" around calls to cut_pool.print_cutpool_types(),
print_cut_types(...), and the pool-size output, and use the logger API rather
than printf when emitting the pool size.
In `@cpp/src/cuts/cuts.cpp`:
- Around line 725-744: The code currently forces verbose logging in the knapsack
detection hot path via the unconditional if (1) that calls settings.log.printf
and the `#if` 1 block printing num_knapsack_constraints; remove these
unconditional logs and instead wrap both the per-row printf and the final count
printf in a runtime verbosity or debug flag check (e.g., settings.verbosity() >
N or settings.log.isDebugEnabled()) so logging only occurs when enabled, and
avoid computing expensive expressions (like sum_pos / (row_len - 1)) unless the
log will be emitted; update the blocks around knapsack_constraints_, the per-row
settings.log.printf call, and the num_knapsack_constraints print to use the
conditional verbosity guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b3fc359d-9dfd-45be-9b16-50160a958299
📒 Files selected for processing (3)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/cuts/cuts.cppcpp/src/cuts/cuts.hpp
…nstraints. Fix variables to reduce the size of seperation problem
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/cuts/cuts.cpp (1)
2058-2112:⚠️ Potential issue | 🟠 MajorGuard the global scaling step in
rational_coefficients().Line 696 now calls this helper on every row. When a row has no integer terms,
gcd()andlcm()both return0, soscalarbecomes NaN and the whole inequality gets scaled by NaN. On the other side, rows with many distinct denominators can overflow the accumulated scale before the laterabs(scalar) > 1000guard ever runs. Please short-circuit the no-integer case and cap the denominator accumulation before applyingscale(). As per coding guidelines "Check numerical stability: prevent overflow/underflow, precision loss, division by zero/near-zero, and use epsilon comparisons for floating-point equality checks".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.cpp` around lines 2058 - 2112, The scaling step in rational_coefficients() must be guarded: if no integer terms were found (indices is empty) return true immediately to avoid gcd/lcm on empty vectors; compute gcd/ lcm only when denominators non-empty. Harden lcm() (and gcd()) to detect overflow during accumulation and clamp the result to a safe maximum (so huge denominators cannot blow up the scalar), and ensure rational_coefficients checks the computed scalar is finite, non-zero and within the allowed bounds (use std::isfinite or equivalent and the existing abs(scalar) > 1000 guard) before calling rational_inequality.scale(scalar). Update function names referenced: rational_coefficients, gcd, lcm, and the vectors numerators/denominators/indices accordingly.
🧹 Nitpick comments (1)
cpp/src/cuts/cuts.cpp (1)
726-745: Please re-hide the knapsack discovery prints.
knapsack_generation_tis constructed unconditionally fromcut_generation_t, soif (1)/#if 1make every cut setup emit per-row diagnostics and the final count even when knapsack cuts are disabled. On large models this will add avoidable overhead in a hot path and flood logs. Prefersettings.log.debug(...)or a dedicated verbose flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.cpp` around lines 726 - 745, The unconditional diagnostic prints around knapsack detection should be hidden: replace the "if (1) { settings.log.printf(...)" block with a debug-level log call (e.g. settings.log.debug(...) or whatever debug API exists) so the per-row message is emitted only when verbose/debug logging is enabled, and change the final "#if 1 ... settings.log.printf("Number of knapsack constraints %d\n", num_knapsack_constraints);" to a debug log as well (using knapsack_constraints_.size() to compute num_knapsack_constraints). Ensure you modify the printf calls around the knapsack_detection block and the final count print to use the debug logging path (or guard them with the existing verbose flag) so diagnostics are no longer emitted unconditionally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/cuts/cuts.cpp`:
- Around line 845-882: The loop can fix all items leaving values.empty(), but
the code still calls solve_knapsack_problem which will crash on empty input;
before calling solve_knapsack_problem check if values.empty(): if so and
seperation_rhs > 0 then set objective = 0 (the reduced knapsack optimum is zero
because any cover must come from fixed_values==1), skip calling
solve_knapsack_problem, then proceed with the existing
isNaN/restore_complemented handling; ensure you reference
solve_knapsack_problem, values, seperation_rhs, fixed_values and
restore_complemented when making the change.
In `@cpp/src/cuts/cuts.hpp`:
- Around line 169-178: The header declares two templates (rational_approximation
and rational_coefficients) but their definitions live only in cuts.cpp and are
not explicitly instantiated, causing unresolved templates for other TUs; either
move the full template definitions of rational_approximation(f_t) and
rational_coefficients<i_t,f_t> into this header (so users including cuts.hpp can
instantiate them), or remove these public declarations and keep the definitions
private in cuts.cpp and/or add explicit template instantiations in cuts.cpp for
every i_t/f_t combination you need; refer to the symbols rational_approximation,
rational_coefficients, variable_type_t and inequality_t in your change to locate
the declarations to update.
---
Outside diff comments:
In `@cpp/src/cuts/cuts.cpp`:
- Around line 2058-2112: The scaling step in rational_coefficients() must be
guarded: if no integer terms were found (indices is empty) return true
immediately to avoid gcd/lcm on empty vectors; compute gcd/ lcm only when
denominators non-empty. Harden lcm() (and gcd()) to detect overflow during
accumulation and clamp the result to a safe maximum (so huge denominators cannot
blow up the scalar), and ensure rational_coefficients checks the computed scalar
is finite, non-zero and within the allowed bounds (use std::isfinite or
equivalent and the existing abs(scalar) > 1000 guard) before calling
rational_inequality.scale(scalar). Update function names referenced:
rational_coefficients, gcd, lcm, and the vectors numerators/denominators/indices
accordingly.
---
Nitpick comments:
In `@cpp/src/cuts/cuts.cpp`:
- Around line 726-745: The unconditional diagnostic prints around knapsack
detection should be hidden: replace the "if (1) { settings.log.printf(...)"
block with a debug-level log call (e.g. settings.log.debug(...) or whatever
debug API exists) so the per-row message is emitted only when verbose/debug
logging is enabled, and change the final "#if 1 ... settings.log.printf("Number
of knapsack constraints %d\n", num_knapsack_constraints);" to a debug log as
well (using knapsack_constraints_.size() to compute num_knapsack_constraints).
Ensure you modify the printf calls around the knapsack_detection block and the
final count print to use the debug logging path (or guard them with the existing
verbose flag) so diagnostics are no longer emitted unconditionally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3015e873-2776-4e76-95da-cda0669f8ff9
📒 Files selected for processing (2)
cpp/src/cuts/cuts.cppcpp/src/cuts/cuts.hpp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cpp/src/cuts/cuts.cpp (1)
2409-2435:⚠️ Potential issue | 🟠 MajorAvoid scaling the whole inequality with a non-finite factor.
If an inequality has no integer coefficients to rationalize—or all approximated integer coefficients are
0—thengcd(...)/lcm(...)return0and Line 2430 computesscalarasNaN/Inf. The function then falls through and scales the entire inequality with that value.Possible fix
for (i_t k = 0; k < input_inequality.size(); k++) { const i_t j = rational_inequality.index(k); const f_t x = rational_inequality.coeff(k); if (var_types[j] == variable_type_t::INTEGER) { int64_t numerator, denominator; if (!rational_approximation(x, static_cast<int64_t>(1000), numerator, denominator)) { return false; } numerators.push_back(numerator); denominators.push_back(denominator); indices.push_back(k); rational_inequality.vector.x[k] = static_cast<f_t>(numerator) / static_cast<f_t>(denominator); } } + + if (numerators.empty()) { return true; } int64_t gcd_numerators = gcd(numerators); int64_t lcm_denominators = lcm(denominators); + if (gcd_numerators == 0 || lcm_denominators == 0) { return true; } f_t scalar = static_cast<f_t>(lcm_denominators) / static_cast<f_t>(gcd_numerators); - if (scalar < 0) { return false; } + if (!std::isfinite(scalar) || scalar <= 0) { return false; } if (std::abs(scalar) > 1000) { return false; }As per coding guidelines "Check numerical stability: prevent overflow/underflow, precision loss, division by zero/near-zero, and use epsilon comparisons for floating-point equality checks".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.cpp` around lines 2409 - 2435, The current code computes scalar = lcm(denominators)/gcd(numerators) without guarding zero/empty cases; if numerators or denominators are empty or gcd_numerators==0 or lcm_denominators==0 the scalar becomes non-finite and you end up scaling rational_inequality with NaN/Inf. Fix by checking after collecting numerators/denominators: if numerators.empty() or denominators.empty() or gcd_numerators == 0 or lcm_denominators == 0 then return false (or skip scaling), and also verify std::isfinite(scalar) before calling rational_inequality.scale(scalar); keep the existing magnitude guard (std::abs(scalar) > 1000) as well.cpp/src/branch_and_bound/branch_and_bound.cpp (1)
2217-2221:⚠️ Potential issue | 🟠 MajorAvoid unconditional cut-pool printing in the cut loop.
Line 2217 forces verbose cut-pool output in every build (
#if 1), which can materially slow large runs and pollute stdout. Keep this behind a debug guard and route output through the logger.💡 Proposed fix
-#if 1 +#ifdef PRINT_CUT_POOL_TYPES cut_pool.print_cutpool_types(); print_cut_types("In LP ", cut_types, settings_); - printf("Cut pool size: %d\n", cut_pool.pool_size()); + settings_.log.printf("Cut pool size: %d\n", cut_pool.pool_size()); `#endif`As per coding guidelines: "Assess algorithmic complexity for large-scale problems (millions of variables/constraints); ensure O(n log n) or better complexity, not O(n²) or worse".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 2217 - 2221, Remove the unconditional debug prints guarded by "#if 1" around cut_pool.print_cutpool_types(), print_cut_types("In LP ", cut_types, settings_), and printf("Cut pool size: %d\n", cut_pool.pool_size()); instead wrap them in a debug/verbose guard (e.g., if (settings_.verbose || ENABLE_DEBUG_LOGGING) or an `#ifdef` DEBUG_CUT_POOL) and route messages through the project's logging facility (replace printf with logger->info/debug/error as appropriate) so cut-pool output only appears when verbose/debug is enabled and does not run in production builds.
♻️ Duplicate comments (1)
cpp/src/cuts/cuts.cpp (1)
872-882:⚠️ Potential issue | 🔴 CriticalHandle the all-fixed separation case before calling
solve_knapsack_problem.After the fixing pass,
valuescan be empty whileseperation_rhsis still positive. Line 881 still callssolve_knapsack_problem, and that routine immediately hitsstd::max_element(values.begin(), values.end())on an empty range. Treat the reduced knapsack optimum as0and continue with the cover induced byfixed_values.Possible fix
if (verbose) { settings.log.printf("Calling solve_knapsack_problem\n"); } - f_t objective = solve_knapsack_problem(values, weights, seperation_rhs, solution); + f_t objective = 0.0; + if (!values.empty()) { + objective = solve_knapsack_problem(values, weights, seperation_rhs, solution); + } else { + solution.clear(); + } if (std::isnan(objective)) { restore_complemented(complemented_variables); return -1; }As per coding guidelines "Validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct results".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.cpp` around lines 872 - 882, The code currently calls solve_knapsack_problem even when values is empty (after fixing) which causes a crash; before calling solve_knapsack_problem check if values.empty() and if so set objective = 0.0 and ensure solution is either left as zeros (solution.resize(values.size()) already does this) and do NOT call solve_knapsack_problem; then proceed to build the cover using fixed_values and complemented_variables as normal (still call restore_complemented(complemented_variables) on the existing early-return paths but not here). Modify the block around seperation_rhs/solve_knapsack_problem to handle values.empty() specially using the function/variable names values, seperation_rhs, solution, solve_knapsack_problem, fixed_values, complemented_variables, restore_complemented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/cuts/cuts.cpp`:
- Around line 946-951: The RHS shift when undoing complementation is wrong:
after calling lifted_cut.negate() the current coefficient at
lifted_cut.vector.x[k] equals -alpha_j, so you must shift the RHS by the
original coefficient alpha_j (rhs -= alpha_j). Fix the loop that handles
complemented variables (where you now do lifted_cut.vector.x[k] *= -1.0;
lifted_cut.rhs += 1.0;) to compute the original coefficient (e.g., old_coeff =
-lifted_cut.vector.x[k]) and apply lifted_cut.rhs -= old_coeff (or equivalently
lifted_cut.rhs += lifted_cut.vector.x[k] if using the post-negation value),
rather than adding 1.0; update the code around lifted_cut.vector.x[k] and
lifted_cut.rhs accordingly.
---
Outside diff comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2217-2221: Remove the unconditional debug prints guarded by "#if
1" around cut_pool.print_cutpool_types(), print_cut_types("In LP ",
cut_types, settings_), and printf("Cut pool size: %d\n", cut_pool.pool_size());
instead wrap them in a debug/verbose guard (e.g., if (settings_.verbose ||
ENABLE_DEBUG_LOGGING) or an `#ifdef` DEBUG_CUT_POOL) and route messages through
the project's logging facility (replace printf with logger->info/debug/error as
appropriate) so cut-pool output only appears when verbose/debug is enabled and
does not run in production builds.
In `@cpp/src/cuts/cuts.cpp`:
- Around line 2409-2435: The current code computes scalar =
lcm(denominators)/gcd(numerators) without guarding zero/empty cases; if
numerators or denominators are empty or gcd_numerators==0 or lcm_denominators==0
the scalar becomes non-finite and you end up scaling rational_inequality with
NaN/Inf. Fix by checking after collecting numerators/denominators: if
numerators.empty() or denominators.empty() or gcd_numerators == 0 or
lcm_denominators == 0 then return false (or skip scaling), and also verify
std::isfinite(scalar) before calling rational_inequality.scale(scalar); keep the
existing magnitude guard (std::abs(scalar) > 1000) as well.
---
Duplicate comments:
In `@cpp/src/cuts/cuts.cpp`:
- Around line 872-882: The code currently calls solve_knapsack_problem even when
values is empty (after fixing) which causes a crash; before calling
solve_knapsack_problem check if values.empty() and if so set objective = 0.0 and
ensure solution is either left as zeros (solution.resize(values.size()) already
does this) and do NOT call solve_knapsack_problem; then proceed to build the
cover using fixed_values and complemented_variables as normal (still call
restore_complemented(complemented_variables) on the existing early-return paths
but not here). Modify the block around seperation_rhs/solve_knapsack_problem to
handle values.empty() specially using the function/variable names values,
seperation_rhs, solution, solve_knapsack_problem, fixed_values,
complemented_variables, restore_complemented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42a18b50-7e3c-4db1-8653-113668d14d65
📒 Files selected for processing (3)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/cuts/cuts.cppcpp/src/cuts/cuts.hpp
chris-maes
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/cuts/cuts.cpp (1)
2430-2436:⚠️ Potential issue | 🟡 MinorPotential division by zero if no integer variables exist.
If the inequality contains no integer variables,
numeratorswill be empty,gcd(numerators)returns 0, and line 2433 will perform division by zero. While knapsack constraints should always have integer variables, adding a guard would make this function more robust.Proposed fix
int64_t gcd_numerators = gcd(numerators); int64_t lcm_denominators = lcm(denominators); + if (gcd_numerators == 0 || lcm_denominators == 0) { return false; } + f_t scalar = static_cast<f_t>(lcm_denominators) / static_cast<f_t>(gcd_numerators); if (scalar < 0) { return false; } if (std::abs(scalar) > 1000) { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.cpp` around lines 2430 - 2436, The code computes scalar using gcd_numerators = gcd(numerators) and then divides by it, which can be zero when numerators is empty; guard this by checking numerators.empty() or gcd_numerators == 0 (from the variables numerators and gcd_numerators) before computing scalar (the static_cast division) and return false (or handle appropriately) to avoid division by zero in the function containing this logic in cuts.cpp.
🧹 Nitpick comments (6)
cpp/src/cuts/cuts.cpp (5)
1018-1019: Typo in method name:minimial_cover_and_partitionshould beminimal_cover_and_partition.Consistent with the header file, this method name has a spelling error.
Proposed fix
template <typename i_t, typename f_t> -void knapsack_generation_t<i_t, f_t>::minimial_cover_and_partition( +void knapsack_generation_t<i_t, f_t>::minimal_cover_and_partition( const inequality_t<i_t, f_t>& knapsack_inequality,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.cpp` around lines 1018 - 1019, The implementation method name knapsack_generation_t<i_t, f_t>::minimial_cover_and_partition is misspelled; rename the definition to knapsack_generation_t<i_t, f_t>::minimal_cover_and_partition to match the header and any declarations. Update any internal references, declarations, or calls (including template instantiations or overrides) that use the old name to the corrected minimal_cover_and_partition so signatures and linkage remain consistent.
1480-1481: Debug logging left unconditionally enabled.The
if (1 || ...)pattern always evaluates to true, causing knapsack cut generation time to always be printed. This appears to be debug code that should be reverted.Proposed fix
f_t cut_generation_time = toc(cut_start_time); - if (1 || cut_generation_time > 1.0) { - settings.log.printf("Knapsack cut generation time %.2f seconds\n", cut_generation_time); + if (cut_generation_time > 1.0) { + settings.log.debug("Knapsack cut generation time %.2f seconds\n", cut_generation_time); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.cpp` around lines 1480 - 1481, The logging for knapsack cut generation is left always-on due to the hardcoded "1 ||" in the condition; update the if-statement (around the block that calls settings.log.printf and uses cut_generation_time) to remove the "1 ||" debug override so it only logs when the intended condition (e.g., cut_generation_time > 1.0) or an appropriate verbose/debug flag is true; ensure you modify the if condition that surrounds settings.log.printf rather than changing the printf call itself.
760-763: Debug logging left unconditionally enabled.The
#if 1directive causes knapsack constraint information to always be printed. This appears to be debug code that should be conditional or removed before merging.Proposed fix
-#if 1 +#ifdef PRINT_KNAPSACK_INFO i_t num_knapsack_constraints = knapsack_constraints_.size(); settings.log.printf("Number of knapsack constraints %d\n", num_knapsack_constraints); `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.cpp` around lines 760 - 763, The debug log printing of knapsack constraints is unconditionally enabled by the `#if` 1 block; remove or gate it behind a proper runtime or compile-time debug flag. Replace the `#if 1` wrapper around the `i_t num_knapsack_constraints = knapsack_constraints_.size(); settings.log.printf("Number of knapsack constraints %d\n", num_knapsack_constraints);` lines with either removal of the printf entirely or a conditional that checks a debug/logging setting (e.g., use existing verbosity/log level or a DEBUG_COMPILE flag) so `settings.log.printf` is only called when debugging is enabled; ensure references to `knapsack_constraints_`, `num_knapsack_constraints`, and `settings.log.printf` are updated consistently.
1040-1051: Consider clearingworkspace_after use to prevent potential stale data issues.The
workspace_vector is populated at line 1042 but not cleared after the function completes. While the current logic correctly only reads values that were set in the same call, adding cleanup would be a defensive measure against future modifications that might inadvertently read stale values.Proposed fix - add cleanup at the end of minimial_cover_and_partition
for (i_t j : cover_indicies) { if (xstar[j] > 1.0 - x_tol) { c2_partition.push_back(j); } else { c1_partition.push_back(j); } } + + // Clear workspace_ to prevent stale data in future calls + for (i_t k = 0; k < knapsack_inequality.size(); k++) { + const i_t j = knapsack_inequality.index(k); + workspace_[j] = 0.0; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.cpp` around lines 1040 - 1051, The workspace_ vector is populated from knapsack_inequality inside minimal_cover_and_partition but never cleared, risking stale values later; after you finish using workspace_ (after building score, cover_indicies, cover_coefficients), reset the entries you wrote—iterate over knapsack_inequality (or over the same indices collected) and set workspace_[j] back to zero (or otherwise clear them) so workspace_ does not retain stale data; reference the workspace_ write site (workspace_[j] = knapsack_inequality.coeff(k)) and perform the cleanup there at function end.
1259-1273: Debug logging left unconditionally enabled in lifting code.Lines 1259 and 1273 unconditionally log information about lifted variables. This should be conditional on a verbose flag or debug mode to avoid excessive output in production.
Proposed fix
if (alpha_k > 0.0) { - settings_.log.printf("Lifted variable %d with alpha %g\n", k, alpha_k); + if (verbose) { settings_.log.printf("Lifted variable %d with alpha %g\n", k, alpha_k); } F.push_back(k); alpha.push_back(alpha_k); values.push_back(alpha_k); weights.push_back(a_k); lifted_cut.vector.i.push_back(k); lifted_cut.vector.x.push_back(alpha_k); } // Remove the variable from the remaining variables and coefficients remaining_variables.pop_back(); remaining_coefficients.pop_back(); } - settings_.log.printf("Lifted %ld variables\n", F.size()); + if (verbose) { settings_.log.printf("Lifted %ld variables\n", F.size()); }Note: You'll need to add
const bool verbose = false;at the start oflift_knapsack_cutsimilar to other methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.cpp` around lines 1259 - 1273, The unconditional debug prints in lift_knapsack_cut (the settings_.log.printf calls that report "Lifted variable ..." and "Lifted %ld variables") should be guarded by a verbose flag; add a local const bool verbose = false at the start of lift_knapsack_cut (matching other methods) and wrap the two settings_.log.printf calls in if (verbose) { ... } so logging only occurs when verbose is enabled; leave the logic that updates F, alpha, values, weights and lifted_cut.vector unchanged.cpp/src/cuts/cuts.hpp (1)
313-318: Typo in method name:minimial_cover_and_partitionshould beminimal_cover_and_partition.The method name contains a spelling error that should be corrected for consistency and readability.
Proposed fix
- void minimial_cover_and_partition(const inequality_t<i_t, f_t>& knapsack_inequality, + void minimal_cover_and_partition(const inequality_t<i_t, f_t>& knapsack_inequality, const inequality_t<i_t, f_t>& negated_base_cut, const std::vector<f_t>& xstar, inequality_t<i_t, f_t>& minimal_cover_cut, std::vector<i_t>& c1_partition, std::vector<i_t>& c2_partition);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.hpp` around lines 313 - 318, The declaration of minimial_cover_and_partition has a typo in its name; rename the function to minimal_cover_and_partition consistently across the codebase (declaration in cuts.hpp and any definitions/usages) to fix the misspelling, update all references (calls, forward declarations, and the corresponding implementation function name) so signature and linkage remain identical (inequality_t<i_t, f_t>& knapsack_inequality, inequality_t<i_t, f_t>& negated_base_cut, const std::vector<f_t>& xstar, inequality_t<i_t, f_t>& minimal_cover_cut, std::vector<i_t>& c1_partition, std::vector<i_t>& c2_partition).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/cuts/cuts.hpp`:
- Around line 340-343: The reset coverage misses clearing workspace_ and
complemented_xstar_ between generate_knapsack_cut() calls on the reused
knapsack_generation_t instance; update the reset pattern so after each
generate_knapsack_cut() invocation you explicitly clear or reinitialize
workspace_ and complemented_xstar_ (analogous to how is_marked_ is reset in
lift_knapsack_cut() and how restore_complemented() handles is_complemented_),
ensuring no helper function reads stale values from workspace_ or
complemented_xstar_ between calls.
---
Outside diff comments:
In `@cpp/src/cuts/cuts.cpp`:
- Around line 2430-2436: The code computes scalar using gcd_numerators =
gcd(numerators) and then divides by it, which can be zero when numerators is
empty; guard this by checking numerators.empty() or gcd_numerators == 0 (from
the variables numerators and gcd_numerators) before computing scalar (the
static_cast division) and return false (or handle appropriately) to avoid
division by zero in the function containing this logic in cuts.cpp.
---
Nitpick comments:
In `@cpp/src/cuts/cuts.cpp`:
- Around line 1018-1019: The implementation method name
knapsack_generation_t<i_t, f_t>::minimial_cover_and_partition is misspelled;
rename the definition to knapsack_generation_t<i_t,
f_t>::minimal_cover_and_partition to match the header and any declarations.
Update any internal references, declarations, or calls (including template
instantiations or overrides) that use the old name to the corrected
minimal_cover_and_partition so signatures and linkage remain consistent.
- Around line 1480-1481: The logging for knapsack cut generation is left
always-on due to the hardcoded "1 ||" in the condition; update the if-statement
(around the block that calls settings.log.printf and uses cut_generation_time)
to remove the "1 ||" debug override so it only logs when the intended condition
(e.g., cut_generation_time > 1.0) or an appropriate verbose/debug flag is true;
ensure you modify the if condition that surrounds settings.log.printf rather
than changing the printf call itself.
- Around line 760-763: The debug log printing of knapsack constraints is
unconditionally enabled by the `#if` 1 block; remove or gate it behind a proper
runtime or compile-time debug flag. Replace the `#if 1` wrapper around the `i_t
num_knapsack_constraints = knapsack_constraints_.size();
settings.log.printf("Number of knapsack constraints %d\n",
num_knapsack_constraints);` lines with either removal of the printf entirely or
a conditional that checks a debug/logging setting (e.g., use existing
verbosity/log level or a DEBUG_COMPILE flag) so `settings.log.printf` is only
called when debugging is enabled; ensure references to `knapsack_constraints_`,
`num_knapsack_constraints`, and `settings.log.printf` are updated consistently.
- Around line 1040-1051: The workspace_ vector is populated from
knapsack_inequality inside minimal_cover_and_partition but never cleared,
risking stale values later; after you finish using workspace_ (after building
score, cover_indicies, cover_coefficients), reset the entries you wrote—iterate
over knapsack_inequality (or over the same indices collected) and set
workspace_[j] back to zero (or otherwise clear them) so workspace_ does not
retain stale data; reference the workspace_ write site (workspace_[j] =
knapsack_inequality.coeff(k)) and perform the cleanup there at function end.
- Around line 1259-1273: The unconditional debug prints in lift_knapsack_cut
(the settings_.log.printf calls that report "Lifted variable ..." and "Lifted
%ld variables") should be guarded by a verbose flag; add a local const bool
verbose = false at the start of lift_knapsack_cut (matching other methods) and
wrap the two settings_.log.printf calls in if (verbose) { ... } so logging only
occurs when verbose is enabled; leave the logic that updates F, alpha, values,
weights and lifted_cut.vector unchanged.
In `@cpp/src/cuts/cuts.hpp`:
- Around line 313-318: The declaration of minimial_cover_and_partition has a
typo in its name; rename the function to minimal_cover_and_partition
consistently across the codebase (declaration in cuts.hpp and any
definitions/usages) to fix the misspelling, update all references (calls,
forward declarations, and the corresponding implementation function name) so
signature and linkage remain identical (inequality_t<i_t, f_t>&
knapsack_inequality, inequality_t<i_t, f_t>& negated_base_cut, const
std::vector<f_t>& xstar, inequality_t<i_t, f_t>& minimal_cover_cut,
std::vector<i_t>& c1_partition, std::vector<i_t>& c2_partition).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c4f10f6a-923f-462e-9038-b017d90ece30
📒 Files selected for processing (2)
cpp/src/cuts/cuts.cppcpp/src/cuts/cuts.hpp
Comment on lines
+340
to
+343
| std::vector<i_t> is_complemented_; | ||
| std::vector<i_t> is_marked_; | ||
| std::vector<f_t> workspace_; | ||
| std::vector<f_t> complemented_xstar_; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that state vectors are properly reset after use in generate_knapsack_cut
# and check constructor initialization
echo "=== Checking constructor initialization ==="
rg -n -A 10 'knapsack_generation_t<i_t, f_t>::knapsack_generation_t' cpp/src/cuts/cuts.cpp
echo ""
echo "=== Checking restore_complemented usage ==="
rg -n -B 2 -A 2 'restore_complemented' cpp/src/cuts/cuts.cpp
echo ""
echo "=== Checking is_marked_ reset ==="
rg -n -B 2 -A 2 'is_marked_\[.*\] = 0' cpp/src/cuts/cuts.cppRepository: NVIDIA/cuopt
Length of output: 2036
🏁 Script executed:
#!/bin/bash
# Find the implementation of restore_complemented
echo "=== Finding restore_complemented implementation ==="
rg -n -A 15 'void restore_complemented' cpp/src/cuts/cuts.cpp
echo ""
echo "=== Checking workspace_ usage ==="
rg -n 'workspace_' cpp/src/cuts/cuts.cpp | head -20
echo ""
echo "=== Checking generate_knapsack_cut signature and entry point ==="
rg -n -A 5 'generate_knapsack_cut.*{' cpp/src/cuts/cuts.cpp | head -30Repository: NVIDIA/cuopt
Length of output: 937
🏁 Script executed:
#!/bin/bash
# Search for restore_complemented in header files
echo "=== Searching for restore_complemented in headers ==="
rg -n 'restore_complemented' cpp/src/cuts/cuts.hpp
echo ""
echo "=== Searching for restore_complemented definition in all cpp files ==="
rg -n -A 10 'restore_complemented\(' cpp/src/cuts/
echo ""
echo "=== Checking if workspace_ is explicitly reset anywhere ==="
rg -n 'workspace_.*=.*clear\|workspace_.*=.*{.*}' cpp/src/cuts/cuts.cpp
echo ""
echo "=== Checking complemented_variables and is_complemented reset ==="
rg -n -B 3 -A 3 'complemented_variables' cpp/src/cuts/cuts.cpp | grep -A 3 -B 3 'clear\|is_complemented_'Repository: NVIDIA/cuopt
Length of output: 5132
🏁 Script executed:
#!/bin/bash
# Check complemented_xstar_ usage and reset
echo "=== Searching for complemented_xstar_ usage ==="
rg -n 'complemented_xstar_' cpp/src/cuts/cuts.cpp
echo ""
echo "=== Check marked_variables scope and full reset pattern ==="
rg -n -B 5 'std::vector<i_t> marked_variables' cpp/src/cuts/cuts.cpp
echo ""
echo "=== Verify workspace_ is not reset within generate_knapsack_cut ==="
rg -n 'workspace_.*=' cpp/src/cuts/cuts.cpp | grep -v 'workspace_('Repository: NVIDIA/cuopt
Length of output: 900
🏁 Script executed:
#!/bin/bash
# Check context around line 942
echo "=== Context around line 942 (complemented_xstar_ usage) ==="
sed -n '935,950p' cpp/src/cuts/cuts.cpp
echo ""
echo "=== Check generate_knapsack_cut function signature ==="
rg -n 'i_t generate_knapsack_cut' cpp/src/cuts/cuts.cpp -A 15
echo ""
echo "=== Check where complemented_xstar_ is populated (line 862 context) ==="
sed -n '855,870p' cpp/src/cuts/cuts.cppRepository: NVIDIA/cuopt
Length of output: 1574
🏁 Script executed:
#!/bin/bash
# Find the full generate_knapsack_cut signature
echo "=== Finding generate_knapsack_cut function signature ==="
rg -n 'i_t generate_knapsack_cut' cpp/src/cuts/cuts.hpp -A 5
echo ""
echo "=== Check where knapsack_generation_t is instantiated ==="
rg -n 'knapsack_generation_t' cpp/src/ --include='*.cpp' --include='*.hpp' | grep -v '//' | head -20
echo ""
echo "=== Check if generate_knapsack_cut is called multiple times ==="
rg -n 'generate_knapsack_cut' cpp/src/ --include='*.cpp' --include='*.hpp' | grep -v 'void\|i_t'Repository: NVIDIA/cuopt
Length of output: 1565
🏁 Script executed:
#!/bin/bash
# Find files that reference knapsack_generation_t
echo "=== Files containing knapsack_generation_t ==="
rg -l 'knapsack_generation_t' cpp/src/
echo ""
echo "=== Check usage of knapsack_generation_t in solver ==="
rg -n 'knapsack_generation_t' cpp/src/solver/solver.cpp | head -20
echo ""
echo "=== Search for multiple calls to generate_knapsack_cut on same instance ==="
rg -n 'generate_knapsack_cut' cpp/src/solver/solver.cppRepository: NVIDIA/cuopt
Length of output: 428
🏁 Script executed:
#!/bin/bash
# List all cpp files in the project
echo "=== Finding all cpp/hpp files ==="
find cpp/src -name '*.cpp' -o -name '*.hpp' | head -30
echo ""
echo "=== Search for knapsack_generation_t instantiation anywhere ==="
rg -n 'knapsack_generation_t<' cpp/src
echo ""
echo "=== Search for generate_knapsack_cut calls ==="
rg -n 'generate_knapsack_cut' cpp/src -B 2 -A 2Repository: NVIDIA/cuopt
Length of output: 4533
🏁 Script executed:
#!/bin/bash
# Check the full context of the reset at lines 1276-1278
echo "=== Context of reset at lines 1276-1278 ==="
sed -n '1270,1285p' cpp/src/cuts/cuts.cpp
echo ""
echo "=== Check if workspace_ is always overwritten before use ==="
sed -n '1019,1055p' cpp/src/cuts/cuts.cpp
echo ""
echo "=== Verify marked_variables is local and reset properly ==="
sed -n '1143,1165p' cpp/src/cuts/cuts.cppRepository: NVIDIA/cuopt
Length of output: 2967
🏁 Script executed:
#!/bin/bash
# Verify if complemented_xstar_ old values could interfere
echo "=== Trace all uses of complemented_xstar_ ==="
rg -n 'complemented_xstar_' cpp/src/cuts/cuts.cpp
echo ""
echo "=== Check if workspace_ values read outside minimial_cover_and_partition ==="
rg -n 'workspace_\[' cpp/src/cuts/cuts.cppRepository: NVIDIA/cuopt
Length of output: 888
Constructor properly initializes state vectors, but reset coverage is incomplete between calls.
The constructor (lines 698-702) correctly initializes all four state vectors to appropriate sizes. However, the reset pattern after each generate_knapsack_cut call is incomplete:
is_complemented_is explicitly reset viarestore_complemented()(called in 6 paths: lines 844, 890, 902, 909, 981, 986)is_marked_is explicitly reset at lines 1276-1278 inlift_knapsack_cut()workspace_is NOT explicitly reset, though values are overwritten at line 1042 before being read at lines 1048, 1050complemented_xstar_is NOT explicitly reset, though values are overwritten at line 862 before being passed to helper functions at line 942
Since the same knapsack_generation_t instance is reused across multiple generate_knapsack_cut() calls in the loop at line 1523, defensively reset workspace_ and complemented_xstar_ after each call (similar to the pattern for is_marked_ at lines 1276-1278) to prevent reliance on overwrite-before-read assumptions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/cuts/cuts.hpp` around lines 340 - 343, The reset coverage misses
clearing workspace_ and complemented_xstar_ between generate_knapsack_cut()
calls on the reused knapsack_generation_t instance; update the reset pattern so
after each generate_knapsack_cut() invocation you explicitly clear or
reinitialize workspace_ and complemented_xstar_ (analogous to how is_marked_ is
reset in lift_knapsack_cut() and how restore_complemented() handles
is_complemented_), ensuring no helper function reads stale values from
workspace_ or complemented_xstar_ between calls.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When looking for 0-1 knapsack cuts, complement variables if the variables coefficient is negative.
This increases the number of problems where knapsack cuts were found on Wolter's test set of 20 MIPs from 5/20 to 15/20.